Conversation
vkurchatkin
added
the
semver-major
|
This is a serious breaking change |
|
ah, yes, good point. The |
jasnell
dismissed
their stale review
Second thoughts... needs some tweaking to avoid the breaking change
|
Sorry to ask this, but can someone explain what exactly is the breakdown? It seems that I something misunderstood :( What the breaking case? |
lib/net.js
Outdated
There was a problem hiding this comment.
If I'm following the code correctly, this could conceivably be a breaking change for some. Since options.fd can be provided by the end user, it may be a string. Before this change, passing in '1' would have the same effect as 1. But now it won't. It only affects whether stdout and stderr are blocking or not on Windows. ¯\(ツ)/¯ @nodejs/platform-windows
There was a problem hiding this comment.
Ah, got it. Thank you!
Hmm, and what are workarounds? ParseInt'ing? (looks like hack :( ) Leave it as is? (options.fd can be === true, thats not okay?)
There was a problem hiding this comment.
Well, you could just leave ==. :-D
options.fd = +options.fd should make it a number too. There are also bit-shifting things I think that might be more efficient.
But ultimately, I'm not sure this needs to be changed at all.
notarseniy
force-pushed
the
net-minor-refactor
branch
3 times, most recently
from
e59eaa4 to
89fa167
Compare
|
Reverted semver-major label now can be removed. Thanks to @vkurchatkin for pointing this out. |
Trott
removed
the
semver-major
lib/net.js
Outdated
There was a problem hiding this comment.
Instead of saying this and linking to a GitHub issue, could you just explain the legacy reasons.
* Change === to == in one place * Add explanation about another non-strict if-statement
notarseniy
force-pushed
the
net-minor-refactor
branch
from
89fa167 to
9209efa
Compare
|
Expanded commit message; made comment on |
* Change === to == in one place * Add explanation about another non-strict if-statement PR-URL: #11513 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
|
Landed in 84c448e |
* Change === to == in one place * Add explanation about another non-strict if-statement PR-URL: nodejs#11513 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
* Change === to == in one place * Add explanation about another non-strict if-statement PR-URL: #11513 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
* Change === to == in one place * Add explanation about another non-strict if-statement PR-URL: #11513 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
* Change === to == in one place * Add explanation about another non-strict if-statement PR-URL: #11513 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
* Change === to == in one place * Add explanation about another non-strict if-statement PR-URL: #11513 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
7 participants
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
net